Skip to content

Make privacy banner not dismissable via WordPress UI tooling #9830

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Jun 1, 2023
Merged

Make privacy banner not dismissable via WordPress UI tooling #9830

merged 4 commits into from
Jun 1, 2023
+10 −17

Conversation

@mokagio
Copy link
Contributor

@mokagio mokagio commented May 29, 2023

Description

Removes the workaround that was required while waiting for the changes in wordpress-mobile/WordPressUI-iOS#126 to be published

Testing instructions

I tested this by forcing the app to present the banner and by both tapping out of it and trying to drag it (in the Simulator) to verify it staid on screen. I'm not familiar with how the component should behave, and we don't have a demo for it in WordPress-UI, so I might have missed something.

I recorded myself during the process, but QuickTime crashed when I tried to crop the video...

For reference, here's the diff I used to force the display:

Diff 
--- a/WooCommerce/Classes/ViewRelated/Dashboard/Settings/Privacy/PrivacyBannerPresenter.swift
+++ b/WooCommerce/Classes/ViewRelated/Dashboard/Settings/Privacy/PrivacyBannerPresenter.swift
@@ -27,16 +27,16 @@ final class PrivacyBannerPresenter {
         guard isUITesting == false else {
             return
         }
+
+//        guard ServiceLocator.featureFlagService.isFeatureFlagEnabled(.privacyChoices) else {
+//            return
+//        }
 
-        guard ServiceLocator.featureFlagService.isFeatureFlagEnabled(.privacyChoices) else {
-            return
-        }
-
-        let useCase = PrivacyBannerPresentationUseCase(defaults: defaults)
+//        let useCase = PrivacyBannerPresentationUseCase(defaults: defaults)
         Task {
-            if await useCase.shouldShowPrivacyBanner() {
+//            if await useCase.shouldShowPrivacyBanner() {
                 await presentPrivacyBanner(from: viewController)
-            }
+//            }
         }
     }
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@mokagio mokagio requested review from Ecarrion and rachelmcr May 29, 2023 05:32
@mokagio mokagio added this to the 13.9 milestone May 29, 2023
@mokagio mokagio added the type: technical debt Represents or solves tech debt of the project. label May 29, 2023
@mokagio mokagio enabled auto-merge May 29, 2023 05:33
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented May 29, 2023 •
edited
Loading

You can test the changes from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr9830-88ed395 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

Ecarrion
Ecarrion reviewed May 29, 2023
View reviewed changes
WooCommerce/Classes/ViewRelated/Dashboard/Settings/Privacy/PrivacyBannerViewController.swift Outdated
// We define them here instead than in the existing extension that defines the protocol conformance so that we can make the stored constants.
// Otherwise, since extensions don't allow for stored properties, they would have had to be computed vars.
let allowsTapToDismiss = false
let allowsDragToDismiss = false
Copy link
Contributor

@Ecarrion Ecarrion May 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we will also need let allowsUserTransition = false to prevent all gestures from dismissing the app.

[Q]: What problem do you see in making these computed vars? I think it may be easier for future maintainers to find all of these properties under the same protocol extension, what do you think? 😅

Copy link
Contributor Author

@mokagio mokagio May 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discoverability is a legit issue 🤔

What problem do you see in making these computed vars?

These values don't change, it seems inefficient to compute them at runtime when they could be set once.

Having said that, if it helps with discoverability, I'm happy to move them in the extension.

Copy link
Contributor Author

@mokagio mokagio May 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 5fc8207

@peril-woocommerce
Copy link

peril-woocommerce bot commented May 31, 2023

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 2 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

@mokagio mokagio requested a review from Ecarrion May 31, 2023 10:53
@Ecarrion Ecarrion self-assigned this May 31, 2023
Ecarrion
Ecarrion approved these changes May 31, 2023
View reviewed changes
Copy link
Contributor

@Ecarrion Ecarrion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks and tests great!
Thanks @mokagio for tackling this change!

@mokagio mokagio force-pushed the mokagio/wordpress-ui-1.13.0 branch from 5fc8207 to 88ed395 Compare June 1, 2023 00:43
@mokagio mokagio merged commit 12d99fa into trunk Jun 1, 2023
@mokagio mokagio deleted the mokagio/wordpress-ui-1.13.0 branch June 1, 2023 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rachelmcr rachelmcr Awaiting requested review from rachelmcr

1 more reviewer

@Ecarrion Ecarrion Ecarrion approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Ecarrion Ecarrion

Labels

type: technical debt Represents or solves tech debt of the project.

Projects

None yet

Milestone

13.9

Development

Successfully merging this pull request may close these issues.

4 participants

@mokagio @wpmobilebot @Ecarrion